SEP-1908: Drive ty's error-severity diagnostics to zero so make typecheck exits 0 - #1447

BaseSQLModelManager._exec declared an unparameterized
TupleResult | ScalarResult | CursorResult, so every consumer's element type
collapsed to 'Unknown | Row[Unknown]' and 21 manager methods across four
modules could not state what they return.

Measured against the pinned sqlmodel 0.0.22: AsyncSession.exec declares exactly
two overloads (SelectOfScalar -> ScalarResult, Select -> TupleResult) and no
DML overload, so a DML statement infers Unknown there. execute() -- which does
return CursorResult -- is @deprecated, and 'deprecated' is an error rule, so it
is not the way out either. _exec therefore takes three overloads: the two
sqlmodel declares, plus a DML arm stating what that path returns at runtime.

Also in the manager layer:

- _QueryBuilder becomes generic in the statement it builds, so _build_query
  returns a Select for the select path and an Update/Delete for the DML path
  rather than an unbound type variable.
- Whereable/Executable name Update and Delete instead of the DMLWhereBase
  mixin, which is not itself a statement type and so appeared to lack
  .options().
- The class-level type variables that no Generic bound (Model, ParentManager,
  AsTypeValidator.validate_class) carry their real types; AsTypeValidator
  becomes generic, which is contained to its eight in-file uses.
- *args: P.args without **kwargs: P.kwargs is not what ParamSpec means; the
  query builders take tuple[Any, ...].

Narrowing _exec revealed two ids read back from the database and returned as
list[int] / dict[int, ...] while typed int | None. BaseSQLModel.id is
nullable=False at the column and optional only before flush, so both read paths
now narrow explicitly.

The package's PEP 562 __getattr__ breaks a genuine import cycle and has to
declare a return of 'object', so every name bound from it was typed 'object'
and could not be used in an annotation.

The two app consumers keep the lazy import for the runtime binding and take the
class itself under TYPE_CHECKING, which is the only branch a checker reads. The
runtime path is byte-for-byte what it was, so the cycle stays broken -- the
clean-interpreter probes in test_import_cycle.py still pass. The two test
modules that only annotate with it import the class directly; they are leaves
and sit outside the cycle.

Fixing this in the package __init__ instead is not available: ruff counts
__all__ as a runtime use, so a TYPE_CHECKING import there trips TC004, and
spelling it as an explicit re-export trips PLC0414.

…verrides are

BaseExecutor.stream_logs and stream_file are annotated
'-> AsyncGenerator[...]' but their bodies are docstring-only, so Python parses
them as plain coroutine functions. The base therefore promised
'Coroutine[..., AsyncGenerator[...]]' while every override -- each containing a
yield -- is a real async-generator function, which is what produced the
override mismatches.

The tell that this is a real defect rather than a cosmetic one is the two
consumers: app/tasks/routes.py and app/tasks/run_result.py both 'async for'
over the result without awaiting it first, so the base was the half that was
wrong. Both bodies now raise NotImplementedError ahead of an unreachable yield,
matching the idiom the Celery override already uses.

stream_logs also widens to 'TaskLog | None'. NomadExecutor yields None for a
step that holds an allocation without emitting lines, and the route consumes it
deliberately -- 'log_line.model_dump_json() if log_line else ""' renders it as
an empty frame that keeps the response open. The None is part of the contract,
so the base now says so rather than the override contradicting it.

…lable()

BaseApp.periodic_task_schedules is already annotated correctly, so the policy
doc's 'fixable by annotating the attribute' does not apply here. Measured
instead: the discriminant is the SkipValidation wrapper. SkipValidation[X] is
Annotated[X, ...], and callable() narrowing through it drops the signature and
yields Top[(...) -> object]; the structurally identical but unwrapped
stop_on_short_page in app/core/pagination/models.py narrows through the same
ternary and reports nothing. Binding to a local first does not help -- only
avoiding callable() does, so the seed path and its test now test for the list
arm.

Two test doubles report the same rule for a different reason: the narrowed
attributes had no declared type. Annotating them fixes the wizard stub. The
FakePopen pair needs slightly more, because a callable is not provably distinct
from a tuple, so no narrowing test on that union yields a clean pair -- the arm
is settled at construction instead and communicate() just calls it.

…ey yield

72 fixtures across 42 files declared the type they yield, so ty read each as a
function returning that type and reported the generator it actually returns.
The annotation is now the generator, in the form each tree already prefers:
AsyncGenerator[X, None] for async (52 existing uses against 10 of the
one-argument form) and Iterator[X] for sync (76 against 3 of
Generator[X, None, None]).

Both AsyncGenerator arities type-check clean at python-version 3.11, so the
choice is consistency, not correctness. No fixture body changes.

Each site subscripts, iterates or operates on a value ty knows may be None. In
tests an explicit 'assert x is not None' is the idiomatic narrowing and
strengthens the test, so that is what these take -- never a silent widening.

Four sites needed something else:

- FakeTaskAPI.last_create_payload was 'dict | None = None' but no consumer ever
  checks for None; all sixteen subscript it directly, so the sentinel only ever
  produced a TypeError. It defaults to an empty dict.
- LOGGING_CONFIG is a dictConfig mapping inferred as a heterogeneous literal, so
  a nested lookup was not subscriptable. It carries the dict[str, Any] the
  settings field of the same name already declares.
- hasattr() does not narrow, so the validation-type test reads its args through
  typing.get_args instead.
- 'hasattr(route, "path") and ... in route.path' becomes
  'getattr(route, "path", "")', which is the same test in a form that narrows.

…astAPI reads it

The provider-selected user class and the derived alters response models are
computed at import time, so a checker cannot use them in an annotation. Where
nothing reads the annotation at runtime -- deps return types, Annotated
dependency aliases -- the annotation is now the static base, which is the
honest ceiling.

Where FastAPI *does* read it, the concrete class is pinned in an explicit
response_model= first. Four routes derive their response model from the return
annotation, so re-annotating them alone would have filtered owner,
is_forbidden and is_deleted out of both the JSON body and the published OpenAPI
schema on GET /api/users/{,me,{username}} and dropped connectivity_warning from
PUT /alters/{task_name}. New tests hold that line from both sides: the body and
the schema are each asserted against the fields the configured provider adds
over BaseUser, and both fail if a response_model= is dropped.

render_alters_create is safe to re-annotate: derive_cascade_create_route takes
its response_model explicitly and never infers one from the builder.

Also here, each an annotation that was simply untrue:

- BaseUser.get_users returned list[Self]; list is invariant, so two overrides
  textually identical to the base were rejected. Sequence[Self] is covariant
  and no caller mutates or index-assigns the result.
- Three alters pre-check helpers were annotated non-optional over bodies that
  return None on failure -- as their own docstrings said, and as every caller
  already guards for.
- get_async_session_maker_from_engine declared async_sessionmaker, imported
  async_sessionmaker, and called sessionmaker. The body now matches; one test
  asserting the legacy class is corrected to the documented contract.
- URL.__get_pydantic_json_schema__ declared GetCoreSchemaHandler for what
  pydantic passes as a GetJsonSchemaHandler.
- ClientRegistry.get returns T but reads a dict[..., BaseRemoteAPI]; the cache
  probe narrows with isinstance now, which the key already guaranteed.
- get_created_entity gains overloads restating ENTITY_MAPPING, so its four
  wrappers get their own model instead of the whole union.

Eleven overrides were incompatible with their bases, in four distinct ways.

Contravariant parameter narrowing -- the override accepted less than the base
promised. The syncers and CasdoorUser.from_token_payload now take the base type
and narrow inside with a guard that raises, which is what makes the inherited
contract true. PagerDutyAlertProvider.send_alert is the same shape, but its
@validate_call was doing the coercion, so the coercion is now explicit and the
base Alert the dispatcher actually passes is what it declares.

PMMSyncer.perform_service_sync gets no guard: it reads only node_id, which the
base Service carries, and a test already calls it with a base Service. A guard
there would have been a live behaviour change, not dead code.

Dropped parameters: BasePeriodicTaskManager.update omitted the base's
**extra_fields; it takes them and forwards them.

Renamed parameters: NodeManager._identity_source renamed session to _session for
ruff ARG003, which is an LSP break. The base's parameters are positional-only
now, so the name stops participating -- both call sites already pass
positionally, and this is the escape the standards prefer over a noqa.

Third-party bases that cannot move: settings_customise_sources narrowed two
sources below what pydantic-settings declares. The three overrides take the base
type and narrow through a structural Protocol naming the one capability the body
needs, env_vars. A nominal isinstance would have failed the existing mocks; the
Protocol accepts them and still rejects a source that carries no env_vars. The
two bytearray test doubles take typeshed's own find/rfind signature.

Eleven reads of a name that ty could not prove was bound. Classified before
fixing, since only a reachable one carries a test obligation -- none of the
eleven turned out to be reachable, and each is now bound locally rather than by
a correlation the reader has to reconstruct.

- Task.data predicates: parent_value was bound under a guard that is the
  disjunction of the two guards reading it, so no path reached an unbound read.
  It builds a SQL expression and does no I/O, so it is bound unconditionally.
- Advisor families: family_suffix was bound and read under the same 'if family'
  and now has a definite binding.
- RemoteAPI.request: response_data is read in the ClientResponseError handler,
  which is only reachable from raise_for_status() -- after the assignment --
  because the only ClientResponseError json() itself raises is ContentTypeError,
  caught by the earlier clause. It is initialized ahead of the try so that
  ordering stops being load-bearing.
- GrafanaUser.from_bearer raised last_error after a loop over a module constant.
  Empty is impossible today; the loop now reports that explicitly instead of
  raising NameError if the constant were ever emptied.
- The valkey payload's dashboard selection is spread over two independent
  blocks and rendered after both. --sentinel defaults on so one always binds,
  but nothing local says so; both names are bound up front and the render is
  guarded on there being graphs to render.
- The parametrized verb test's if/elif chain covers every parametrized method
  and now fails loudly on one that is added without a branch.

The long tail of per-site defects. Grouped by what was actually wrong:

Annotations that denied a nullability the code has. RemoteAPI.session declared
ClientSession over a value that is None before open() and after close() --
which is exactly what run_result.py and four tests check it for. The two
__aexit__ overloads declared their three parameters non-optional, but the
protocol passes None on a clean exit.

Annotations that over-claimed an element type. get_children_entities and
_schema_form_fields declared a narrower element than their sources yield;
can_sync_mapping is a heterogeneous dispatch table whose key is what makes a
lookup well-typed, and its docstring already said so.

Reads of a JSON payload as a concrete shape, wrapped in as_json_object like the
rest of the ticket.

Genuine narrowing gaps: getattr(self, name, None) does not narrow the
attribute; a Grafana account id read out of an untyped payload is checked as an
int now rather than merely not-None; a storage config keyed on an absent
storage_type would have produced a nonsense mapping.

_TTLCache.get returned (hit, value), a correlation no signature can express --
and one that a cached None would have broken on its own. It raises KeyError on
a miss, which is the shape dict lookup already has.

The two dipper payload mains return an exit code and now say so, and
coerce_target_list is annotated as the before-validator it is, matching
coerce_footer_template.

The remainder of the per-site work under app/ and sidecar/.

update_where and delete_where always materialize a list when 'returning' is
truthy -- both the RETURNING path and the MySQL FOR UPDATE workaround -- and a
Result otherwise, so overloads let their callers state which they asked for
instead of re-declaring the union and being wrong.

Four functions return a *type expression* assembled at runtime -- an
Annotated[...] form, a runtime StrEnum, an 'X | None' union -- none of which is
a 'type'. They say Any and name the reason, which is the honest ceiling; the
choice enum moves into a helper so its own name no longer has to match the
variable it lands in.

The MySQL syncer's schemas_index getter declared '| None' but substitutes an
empty iterator, so it never returns None.

check_mongodb caught pymongo.errors.OperationFailure while importing only the
package; importing the package does not bind the submodule. It imports both
now, and the fixture that injects a mock pymongo registers both entries in
sys.modules to match -- the same reason the real code needs the explicit
import.

Two routes returned a table model where a response model is declared and let
FastAPI convert; they build the declared model, so the annotation is true in
the source. The PBM payload preamble is regenerated for the credentials-path
narrowing (make regen-pbm-payloads).

…gnore

The tail of the test-side work.

_bind_suite assigned app_def on an instance, but it is a ClassVar -- the suite
documents itself as being subclassed with it bound, so the helper builds that
subclass instead.

_make_validation_error passed a 'msg' key to InitErrorDetails, which that
TypedDict does not carry; pydantic was ignoring it.

The masking test derived a model with type(name, (model,), ...), a class base
taken from a variable. pydantic's own create_model does the same thing and is
the idiom for a derived model.

The rest are the usual narrowing: a dependency callable that may be None, a
mock's await_args, a payload function extracted out of an exec namespace, a
manager's optional ordering. _build_syncer is generic in the syncer class it is
handed, so a caller naming StubTestSyncer gets one back.

test_manager reached AsyncSession.execute, which sqlmodel deprecates and the
policy treats as an error; it uses exec. The fetch_page stub drops a mypy-syntax
'type: ignore' that suppressed nothing under ty and annotates what it returns
now that fetch_all_dict_items accepts a raw page.

…e policy

Two diagnostic shapes have no fix available in this repository, so they take the
mechanism SEP-1906 built for exactly that: a Group in
classify_ty_diagnostics.py naming the discriminant, and the per-site comments
its report prescribes. Per-site rather than a [[tool.ty.overrides]] entry
because an override suppresses the rule for a whole file, and a genuinely broken
override written into one of these files later would never be reported.

predicate-dsl-comparison-operators -- FieldExpr.__eq__/__ne__ return a Predicate
so F(field) == value builds a rule node, as SQLAlchemy does for a column.
object.__eq__ is declared '-> bool' in typeshed and cannot move. The comments
they carried were mypy syntax and suppressed nothing under ty.

runtime-computed-model-in-type-position -- 17 annotations naming a class chosen
at runtime: the provider-selected user model, create_model-derived response
models, form models the framework reads back through get_type_hints. Python has
no spelling for 'the class in this variable'; measured against ty 0.0.49, the
only construction that avoids the diagnostic is list.__class_getitem__(model),
which covers neither the annotation positions nor PaginatedResponse[...], and
reads as a dodge rather than a fix.

ty-policy.md records the re-measured baseline -- 3,178 diagnostics, 0 error,
make typecheck exit 0 -- and gains the trigger its change-policy list was
missing: clearing diagnostics in bulk moves that figure the same way a ty
upgrade does. The narrowing helpers are registered in existing-patterns.md.

Appending the directive lengthened those lines past the formatter's limit, so
ruff-format wrapped them and the comment landed one line below the diagnostic --
unused where it sat, missing where it was needed. Inside the wrapped brackets
the placement is stable.

C19 wants the element type on an empty-collection assignment; C1 wants no local
annotation where inference suffices, so the query merge is one expression with
the same precedence the update() call had; C11 wants no new inline import, and
'import pymongo.errors' binds pymongo on its own, so the module import it
replaces is not needed.

The gates read the committed diff, so every docstring line this change touched
came back as newly added and had to meet the current conventions rather than
the ones its neighbours were written under.

Optional :type:/:rtype: directives are dropped from the lines this change
edited -- the annotation is the source of truth -- and the three alters
pre-check docstrings move from Google style to the rST the project uses. The
query builders and _exec document their parameters and returns. A spaced '--'
becomes an em dash.

Two deferrals, both using the marker the gate itself prescribes:

- The users listing has no upstream window to page against: both provider SDKs
  return the whole organization in one call.
- Thirteen fixtures duplicate a database bootstrap that predates this change,
  which only re-annotated their return types; promoting them is a cross-tree
  refactor. Same for the three query builders, which are already the thin
  wrappers the repeated-call-shape rule asks for.

The provider-field tests gained a positive control: the derived field set is
empty under Grafana, so the subset assertions would have passed vacuously
there -- the vacuous-assertion gate caught a real hole in them.

The narrowing helpers reject a mis-shaped upstream JSON body with
HTTPBadGatewayException, so a caller that previously saw a 500 (or silently
wrong data) now sees a 502 carrying a detail. Every other change in this ticket
is annotation-only and internal.

No deployment asymmetry and no new operator precondition: the change is in the
shared HTTP client helpers and reads no configuration.

Review of the typecheck-narrowing work found three places where the newly
declared contract was not the one the code honours.

- `update_where` / `delete_where` promise `list[Any]` for any non-bool
  `returning`, but `_dml_where` branches on truthiness, so `returning=[]`
  returned a `CursorResult`. Reject an empty `returning` instead, so the
  overloads state the truth.

- `delete_where` forwarded a vestigial `values=None` that `_dml_where` has no
  parameter for, so it landed in `**equal_filters` and made the "at least one
  filter" guard unreachable for every DELETE: `delete_where(session)` truncated
  the table. Pre-existing on main; every real caller already passes a filter,
  so dropping the argument only closes the hole.

- `send_alert` kept `@validate_call` after being widened to the base `Alert`,
  which coerced a mapping argument to `Alert` first and rejected the lowercase
  PagerDuty severities. Drop the decorator; the explicit conversion in the body
  already validates.

Also harden `download_task_history_file`, whose best-effort metadata lookup
called `.get` on an unnarrowed JSON body, and return the payload itself from
`as_json_object` / `as_json_array` rather than copying it on every call.

Claude-Session: https://claude.ai/code/session_01XBazM9VWrYcgSrJoQszTNT

…e a docstring

`check_list_pagination` scans one line above the first decorator, so a
three-line `pagination-ok:` block above `@router.get` put the pragma token out
of range. Move it inside the decorator, next to the `response_model` it
justifies.

Adding a `:raises:` to `_dml_where` turned its docstring into a structured one,
which `check_docstring_hygiene` then holds to full param and return coverage.
Document the rest.

Claude-Session: https://claude.ai/code/session_01XBazM9VWrYcgSrJoQszTNT

Claude-Session: https://claude.ai/code/session_01XBazM9VWrYcgSrJoQszTNT

…count

The spec embeds route docstrings, and `list_users` lost the `:rtype:` line its
new `response_model=` makes redundant. The schema itself is byte-identical,
which is the confirmation that pinning the response model preserved the
published contract exactly.

The stale-group assertion wrote down a count equal to every registered group
but the one its fixture matches, so adding a group failed it for a reason the
test is not about. Derive it from `GROUPS` instead.

Claude-Session: https://claude.ai/code/session_01XBazM9VWrYcgSrJoQszTNT

…h from_bearer grew

Two findings from the review pass over this branch.

- `_build_query` parameterized `builder` on `W` while defaulting it to a
  concrete `_QueryBuilder[Select[Any]]`, which `ty` reports as
  `invalid-parameter-default` — a diagnostic this branch introduced, since the
  parameter was unparameterized before. Nothing solves `W` when `builder` is
  omitted, so the two list-path call sites got `Unknown` back and the `_exec`
  overloads added alongside could not key on the statement shape, which is what
  they exist for. Overloads split the two cases the way `_exec`, `update_where`
  and `delete_where` already do in this file. The three sibling signatures that
  still spelled `_QueryBuilder` bare become explicit.
- `from_bearer`'s new `if last_error is None` arm can raise `GrafanaException`,
  which its `:raises` list did not mention. The arm is unreachable while
  `_BEARER_TOKEN_TYPES` is a non-empty literal, so the docstring says that
  rather than a test asserting a state the module cannot reach.

Claude-Session: https://claude.ai/code/session_01R91BB414V223kzWimUqENs

…ommit

The recorded figure is a live claim about the current configuration, so it has
to name the tree it was taken from. It read 3,178 against a tree reporting
3,175 before the review fixes and 3,169 after — the drift the change-policy
entry added just below it exists to catch. Naming the commit is what lets the
next reader tell a stale figure from a current one.

Claude-Session: https://claude.ai/code/session_01R91BB414V223kzWimUqENs

Five files conflicted, plus one silent auto-merge hybrid.

- `app/core/db/crud.py`: main dropped MySQL as a supported engine, taking
  `DatabaseDialect.MYSQL` and the `_mutate_where_returning_with_for_update`
  workaround with it. Took main's deletion and kept this branch's
  `returning` materialization guard; the docstrings that described the
  dialect branch no longer do.
- `app/core/requests/remote_api.py`: both sides added module-level helpers at
  the same point. Union — `as_json_object`/`as_json_array` and
  `is_non_json_success` all survive, and `__all__` names all three.
- `app/core/utils/fields.py`, `tests/app/core/requests/test_remote_api.py`:
  took main's enum rename and the union of both import lists.
- `tests/app/conftest.py`: took main's removal of the real-MySQL fixtures.
- `app/sep/bundle_upload/factory.py` auto-merged into a wrong hybrid —
  main's widened `Mapping` parameter over this branch's `dict | query` body,
  which raises `TypeError` for any non-dict mapping. Restored main's body and
  annotated the local so the merged value type is the declared one.

Two error-severity diagnostics arrived with main, which had not been held to
the zero-error bar: the `|` merge above, and a comprehension iterating an
`aioresponses` mapping whose class-level default is `None`. The latter now
iterates `.items()` like the other fourteen sites in that file, and counts one
entry per recorded request rather than per distinct key.

Merging main added 32 warning-severity diagnostics, which moves the recorded
figure the same way clearing diagnostics in bulk does. The error count stays at
zero: the two errors that arrived with main were fixed in the merge commit.

Co-authored-by: yyyyyyyan <24644216+yyyyyyyan@users.noreply.github.com>